Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: avoid auto_provisioning_defaults drift #1806

Conversation

michaellzc
Copy link
Contributor

@michaellzc michaellzc commented Nov 23, 2023

fixes #1596

This PR added two new optional fields to var.cluster_autoscaling, enable_secure_boot=false, and enable_integrity_monitoring=true.

design decisions:

  • enable_integrity_monitoring defaults to true, which is what other places are using
  • enable_secure_boot defaults to false, which is what other places are using
  • I decided not to reuse attribute from lookup(var.node_pools[0], "enable_integrity_monitoring", true), similar pattern was used for auto_provisioning_defaults.oauth_scopes, and it was actually a surprise to me. It's better to give users more control over it.

Why not just let provider to decide this optional field?

Since some google provider version ago, cluster in this module with NAP enabled will result in infinite drift, and GCP may or may not decided to update the cluster (it's unpredictable)

  # module.gke_self_15C14BFE.google_container_cluster.primary will be updated in-place
  ~ resource "google_container_cluster" "primary" {
        id                          = "projects/<redacted>/locations/us-central1/clusters/<redacted>"
        name                        = "<redacted>"
        # (31 unchanged attributes hidden)

      ~ cluster_autoscaling {
            # (2 unchanged attributes hidden)

          ~ auto_provisioning_defaults {
                # (5 unchanged attributes hidden)

              - shielded_instance_config {
                  - enable_integrity_monitoring = true -> null
                  - enable_secure_boot          = false -> null
                }

                # (2 unchanged blocks hidden)
            }

            # (2 unchanged blocks hidden)
        }

        # (27 unchanged blocks hidden)
    }

Test:

Using our own fork with this patch https://github.com/michaellzc/terraform-google-kubernetes-engine/tree/fork-26-1-1,

terraform plan no longer showed any drift at auto_provisioning_defaults .shielded_instance_config.

@michaellzc michaellzc requested review from ericyz and a team as code owners November 23, 2023 19:04
michaellzc added a commit to sourcegraph/controller-cdktf that referenced this pull request Nov 23, 2023
@michaellzc michaellzc force-pushed the mlzc/add-more-opts-to-auto-provisioning-defaults branch from 0bf17d2 to bc1c27b Compare November 23, 2023 22:41
Comment on lines 252 to 253
disk_size = optional(number, 100)
disk_type = optional(string, "pd-standard")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the default values for optional needs to be provided in the modifier, otherwise, terraform will still require the attribute.

Copy link

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days

@github-actions github-actions bot added the Stale label Jan 22, 2024
@michaellzc
Copy link
Contributor Author

renew

@apeabody apeabody removed the Stale label Jan 22, 2024
Copy link

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days

@github-actions github-actions bot added the Stale label Mar 23, 2024
@michaellzc
Copy link
Contributor Author

renew

@apeabody apeabody removed the Stale label Mar 28, 2024
Copy link

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days

@github-actions github-actions bot added the Stale label May 27, 2024
@apeabody apeabody removed the Stale label May 30, 2024
Copy link
Contributor

@apeabody apeabody left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @michaellzc!

Can you please rebase/re-build, and we'll get a fresh run of the CI tests.

@michaellzc michaellzc force-pushed the mlzc/add-more-opts-to-auto-provisioning-defaults branch from bc1c27b to 43a1d7e Compare May 30, 2024 20:40
@michaellzc michaellzc requested a review from gtsorbo as a code owner May 30, 2024 20:40
@michaellzc
Copy link
Contributor Author

michaellzc commented May 30, 2024

Thanks for the contribution @michaellzc!

Can you please rebase/re-build, and we'll get a fresh run of the CI tests.

@apeabody
It's done. Would you kick off Google's internal CI?

@apeabody
Copy link
Contributor

/gcbrun

@apeabody apeabody dismissed their stale review May 31, 2024 00:01

stale review

@michaellzc
Copy link
Contributor Author

Thanks for the contribution @michaellzc!
Can you please rebase/re-build, and we'll get a fresh run of the CI tests.

@apeabody It's done. Would you kick off Google's internal CI?

@apeabody Thank you.

All CI checks pass now. Is there anything I can do to help move things forward?

@apeabody apeabody self-assigned this Jun 4, 2024
Copy link
Contributor

@apeabody apeabody left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @michaellzc!

enable_secure_boot = false and enable_integrity_monitoring = true matches TPG defaults, and are GA in minimum TPG version constraint

@apeabody apeabody merged commit 0005ab9 into terraform-google-modules:master Jun 5, 2024
4 checks passed
CPL-markus pushed a commit to WALTER-GROUP/terraform-google-kubernetes-engine that referenced this pull request Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unable to update auto_provisioning_defaults shielded_instance_config
2 participants